Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] New router #215

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

[WIP] New router #215

wants to merge 9 commits into from

Conversation

rgrinberg
Copy link
Owner

@rgrinberg rgrinberg commented Nov 13, 2020

TODO

  • trie based implementation
  • add **
  • port old router
  • documentation
  • optimization
  • proper error messages
  • tail recursive addition of routes. let's skip this for now, I doubt this will be useful for realistic routes

Discussion

  • Do we need optional parameters? E.g. :foo?

  • I still haven't created an api for site composition. Still thinking of what is
    that going to look like

@rgrinberg
Copy link
Owner Author

Okay, I think this is ready for some preliminary review.

@joseferben
Copy link
Collaborator

Regarding API for site composition, I implemented a very simple router API like this for Sihl:

let all =
  [
    router ~scope:"" ~middlewares:Middleware.site_middlewares public;
    router ~scope:"/applicants"
      ~middlewares:Middleware.logged_in_site_middlewares applicant;
    router ~scope:"/applications"
      ~middlewares:Middleware.applications_site_middlewares application;
    router ~scope:"/admin" ~middlewares:Middleware.admin_middlewares admin;
    router ~scope:"/api" ~middlewares:[] api;
  ]

But it became apparent that we need an API that allows nesting routers. F#'s Giraffe has these combinators.

let notLoggedIn =
    RequestErrors.UNAUTHORIZED
        "Basic"
        "Some Realm"
        "You must be logged in."

let mustBeLoggedIn = requiresAuthentication notLoggedIn

let webApp =
    choose [
        route "/"     >=> text "Hello World"
        route "/user" >=>
            mustBeLoggedIn >=>
                choose [
                    GET  >=> readUserHandler
                    POST >=> submitUserHandler
                ]
    ]

@rgrinberg
Copy link
Owner Author

rgrinberg commented Nov 15, 2020

That looks interesting. What I had in mind with composition was the ability to "mount" 3rd party web apps under a URL. Using something like:

val mount : Router.t -> string -> Rock.App.t -> Router.t

For example:

let router = mount router "/metrics" metrics in

The idea is that all the routers inside metrics will now be relative to /metrics prefix now.

What your snippet shows is more of an API that allows for hierarchical construction of the router. What I mean by that is that parameters are transparent to the subroutes. E.g.:

let webApp =
    choose [
        route "/user/:id" >=>
            mustBeLoggedIn >=>
                choose [
                    GET  >=> readUserHandler
                    POST >=> submitUserHandler
                ]
    ]

:id should be visible to both routes, correct?

@@ -1,3 +1,9 @@
module Private : sig
module Router : module type of struct
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, module type of Router wouldn't be the same thing? Or is this to allow adding other elements to the signature?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall that the simplest form omitted some type equalities for types inside Router.t. So if one used Router from outside this alias, the type Router.t wouldn't be equal. But I think this bug was fixed not too long ago. I'll see if the simpler form works as well.

val m : Rock.Handler.t t -> Rock.Middleware.t
val empty : 'action t
val add : 'a t -> route:Route.t -> meth:Method.t -> action:'a -> 'a t
val m : t -> Rock.Middleware.t
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any guidelines on naming conventions for similar functions in Opium? I assume m is "middleware", but would be good to know if this should be used more generally.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's my original naming convention for a module that provides a middleware. It seemed a little long to name things like Middleware_foo.middleware. Not so sure if this convention is still beneficial.

@rizo
Copy link
Collaborator

rizo commented Nov 16, 2020

Haven't looked in detail into this, but one thing that I believe is important is to expose the ability to iterate and inspect routes after they have been added. The use case that I have in mind is generating OpenAPI/Swagger specifications for the API.

@rgrinberg
Copy link
Owner Author

Haven't looked in detail into this, but one thing that I believe is important is to expose the ability to iterate and inspect routes after they have been added. The use case that I have in mind is generating OpenAPI/Swagger specifications for the API.

This will be covered. Though I think we'll need a handler type that allows for more introspection.

@rgrinberg rgrinberg force-pushed the new-router branch 4 times, most recently from a31396c to 6a8aff7 Compare December 28, 2020 00:48
@rgrinberg
Copy link
Owner Author

I'm now wondering how to treat trailing slashes and parameter capture. Should * capture empty parameters? For example, should /foo/:bar capture /foo//? Additionally, should /** distinguish /foo/ from /foo?

@tmattio
Copy link
Collaborator

tmattio commented Dec 28, 2020

Should * capture empty parameters? For example, should /foo/:bar capture /foo//

I would say no, IMO /foo/ should be the same as /foo//, and I would expect that /foo/:bar does not capture /foo/.

Additionally, should /** distinguish /foo/ from /foo

Again, I would say no, but it's a bit more opinionated. I remember some websites where the former redirected to the latter for instance. FWIW, I just tried on Github and it does not make a distinction. That's sensible behavior IMO, and the end-users are always free to implement their own Router, so it's ok to be opinionated I think.

@reynir
Copy link
Contributor

reynir commented Dec 28, 2020

I think it makes sense to remove empty segments (save for / I guess). It's illegal to specify a route matching an empty segment:

then raise (E "Double '/' not allowed")

I don't know what the behavior is with this implementation, but with the current implementation /foo/:bar/ matches /foo/bar/ but not /foo/bar. Meanwhile, /foo/** treats /foo/bar/ and /foo/bar the same. I think that's a bit surprising.

I am using full splat for serving files from an archive file: /:archive/file/**, and I think it's interesting that both /foo/file/README.txt and /foo/file/README.txt/ work.

@rgrinberg
Copy link
Owner Author

rgrinberg commented Dec 28, 2020

I would say no, IMO /foo/ should be the same as /foo//, and I would expect that /foo/:bar does not capture /foo/.

That makes it impossible to have something like /foo/:bar/:baz where a route only fills :baz but not :bar. Probably not too big of a deal, but not very elegant. Are you familiar with how other frameworks handle this?

That's sensible behavior IMO, and the end-users are always free to implement their own Router, so it's ok to be opinionated I think.

That reminds me that ** is how I expect users to implement their own router while staying compatible with the rest of the ecosystem. Currently, we just merge * and **, but that needs to change. @reynir are you ok with that?

EDIT:

In particular, if we treat ** opaquely and just pass remaining the user, then you'll be able to handle

I am using full splat for serving files from an archive file: /:archive/file/**, and I think it's interesting that both /foo/file/README.txt and /foo/file/README.txt/ work.

As you prefer.

@reynir
Copy link
Contributor

reynir commented Dec 28, 2020

I would say no, IMO /foo/ should be the same as /foo//, and I would expect that /foo/:bar does not capture /foo/.

That makes it impossible to have something like /foo/:bar/:baz where a route only fills :baz but not :bar. Probably not too big of a deal, but not very elegant. Are you familiar with how other frameworks handle this?

In my (limited) experience it most often doesn't matter if you add empty segments, e.g.: https://github.com/rgrinberg//opium///pull/215

That's sensible behavior IMO, and the end-users are always free to implement their own Router, so it's ok to be opinionated I think.

That reminds me that ** is how I expect users to implement their own router while staying compatible with the rest of the ecosystem. Currently, we just merge * and **, but that needs to change. @reynir are you ok with that?

EDIT:

In particular, if we treat ** opaquely and just pass remaining the user, then you'll be able to handle

I am using full splat for serving files from an archive file: /:archive/file/**, and I think it's interesting that both /foo/file/README.txt and /foo/file/README.txt/ work.

As you prefer.

Yes, I like this.

@tmattio
Copy link
Collaborator

tmattio commented Dec 29, 2020

That makes it impossible to have something like /foo/:bar/:baz where a route only fills :baz but not :bar. Probably not too big of a deal, but not very elegant. Are you familiar with how other frameworks handle this?

One expectation I have in the handler is that the parameter is present, so for this example, I would not expect the route to be matched if one of the parameters is absent.

I'm looking at Rails documentation, for which the router DSL is similar to Opium's and they have two kinds of parameters:

  • :foo that is required and must be non-empty
  • (:bar) that is optional and can be empty

We could do something similar?

Now considering the limitation you mention: if the parameter is optional, I would expect something/(:bar)/:foo to be invalid. But I'm not sure how Rails or other frameworks deal with it, I could make some research.

I am using full splat for serving files from an archive file: /:archive/file/**, and I think it's interesting that both /foo/file/README.txt and /foo/file/README.txt/ work.

That seems fine to me. ** should match anything. If the intended behaviour is to only match one segment, /:archive/file/* would make more sense no?

Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants